Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] AIOps: Adds expanded rows to pattern analysis table #175320

Conversation

jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Jan 23, 2024

Changes the syntax highlighting of the example text so that it is now based on the pattern's tokens, rather than being the default "log" format supplied by EuiCode
Adds an expanded row to the able which lists the tokens, the regex and 4 examples so we can see how the non-token parts of each example vary.

Highlighting colors have been copied from the EuiCode component.

image

@jgowdyelastic
Copy link
Member Author

/ci

@jgowdyelastic
Copy link
Member Author

/ci

@jgowdyelastic jgowdyelastic self-assigned this Jan 24, 2024
@jgowdyelastic jgowdyelastic added release_note:enhancement :ml Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis v8.13.0 labels Jan 24, 2024
@jgowdyelastic jgowdyelastic changed the title [ML] Adding expanded rows to pattern analysis table [ML] [AIOps] Adding expanded rows to pattern analysis table Jan 24, 2024
@jgowdyelastic jgowdyelastic marked this pull request as ready for review January 24, 2024 17:32
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner January 24, 2024 17:32
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvement! Left a view comments:

isExpander: true,
render: (item: Category) => (
<EuiButtonIcon
data-test-subj="aiopsColumnsButton"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Suggest to use aiopsLogPatternsColumnsButton.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 5165088


export const ExpandedRow: FC<ExpandedRowProps> = ({ category }) => {
return (
<div css={{ marginRight: '40px', width: '100%' }}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to break that out and use EUI theme vars, something like:

import { css } from '@emotion/react';
import { useEuiTheme } from '@elastic/eui';

export const ExpandedRow: FC<ExpandedRowProps> = ({ category }) => {
    const { euiTheme } = useEuiTheme();

    const cssExpandedRow = css({
        marginRight: euiTheme.size.xxl, // =40px
        width: '100%'
  });

  return (
    <div css={cssExpandedRow}>
...

We use a similar pattern in x-pack/plugins/aiops/public/components/mini_histogram/mini_histogram.tsx.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 5165088

Comment on lines 57 to 59
expect(<FormattedPatternExamples category={category} />).toMatchSnapshot();
expect(<FormattedRegex category={category} />).toMatchSnapshot();
expect(<FormattedTokens category={category} />).toMatchSnapshot();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to suggest that we try to avoid relying on snapshot testing for new unit tests. A while ago we also documented old tests that we wanted to refactor and no longer rely on snapshot tests (#153288). The snapshots render the component with its props but do not assert what's actually rendered.

An alternative with react-testing-lib would look something like:

import {render, screen} from '@testing-library/react';
import '@testing-library/jest-dom';

...

describe('Format category components', () => {
    it('renders FormattedPatternExamples correctly', async () => {
      render(<FormattedPatternExamples category={category} />);
      expect(screen.getByText(/some text we expect to be rendered/i)).toBeInTheDocument();
    });
    ...
});

You could also adapt this to assert just certain pattern elements you'd expect to be rendered within span elements for the custom highlighting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I'm really interested here is the number of elements created, as in, how the line has been split up.
My original version of this test did just that, I'd count the elements and ensure that the line had been split up correctly.
Then I noticed that I'd get the same result by using snapshot.
I can revert this to counting the elements. Shame I deleted the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the test in ff9785b
Reverting it back to a test to count the number of elements returned from createFormattedExample

return elements;
}

return { createFormattedExample };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This could be just return createFormattedExample;.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 5165088

const tokenStyleLight = css`
color: #765b96;
`;
const wildcardStyleLight = css`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about dropping the font-weight for the wildcard style? With equal weights given to the tokens and wildcards, visually the tokens don't stand out from the variable parts of the message for me.

This is with font-weight: 500 for example:

image image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to change stying here, but before I do I think it's worth discussing which parts of the text do we consider to be more important?
The tokens, i.e. the shared words in each example, or the wildcard matches, i.e. the text which differs in each example?
I think there is an argument that the varying parts of the examples are more (or equally) interesting to the user. They'd want to see why these examples are different to each other.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes good point. It was just an initial impression. I did think the tokens were more important as we're displaying patterns here, but highlighting the variable parts of the messages is also valuable. Happy to stick with this for now and see if there's any future feedback.

…ub.com:jgowdyelastic/kibana into adding-expanded-rows-to-pattern-analysis-table
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and LGTM. A great enhancement!

Could we use this same styling / approach in the other parts of the UI where we display similar information?

For example the expanded row in the anomalies table for categorization jobs:

image

And the categorization job wizard:

image

Comment on lines 40 to 48
return isDarkTheme
? {
tokenStyle: tokenStyleDark,
wildcardStyle: wildcardStyleDark,
}
: {
tokenStyle: tokenStyleLight,
wildcardStyle: wildcardStyleLight,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This could be wrapped in a useMemo so it returns the same instance on every call as long as isDarkTheme doesn't change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in c45c849

export const useCreateFormattedExample = () => {
const { tokenStyle, wildcardStyle } = useStyles();

function createFormattedExample(key: string, example: string): JSX.Element[] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This function could be wrapped in useCallback() to avoid returning a new instance every time. Otherwise I think the useMemo in FormattedPatternExamples might not memoize as intended.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in c45c849

};

const WrapInText: FC = ({ children }) => (
<EuiText css={{ fontWeight: 'bold' }} size="s">
Copy link
Contributor

@walterra walterra Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This css could be broken out to a definition outside the component scope. AFAIK the benefit would be that internally this would then be treated as a css class instead of inline css on every element.

Copy link
Member Author

@jgowdyelastic jgowdyelastic Jan 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell from testing, this still creates the class which is shared between all rows.
I believe this is why we were encouraged to use inline css= rather than style= for jsx.

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some more minor suggestions but otherwise LGTM, also did a local test.

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 427 429 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 394.4KB 397.8KB +3.4KB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jgowdyelastic

@jgowdyelastic jgowdyelastic merged commit b10f083 into elastic:main Jan 26, 2024
18 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 26, 2024
@peteharverson peteharverson changed the title [ML] [AIOps] Adding expanded rows to pattern analysis table [ML] [AIOps] Adds expanded rows to pattern analysis table Feb 14, 2024
@peteharverson peteharverson changed the title [ML] [AIOps] Adds expanded rows to pattern analysis table [ML] AIOps: Adds expanded rows to pattern analysis table Feb 14, 2024
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this pull request Feb 15, 2024
…175320)

Changes the syntax highlighting of the example text so that it is now
based on the pattern's tokens, rather than being the default "log"
format supplied by `EuiCode`
Adds an expanded row to the able which lists the tokens, the regex and 4
examples so we can see how the non-token parts of each example vary.

Highlighting colors have been copied from the `EuiCode` component.

<img width="1027" alt="image"
src="https://github.com/elastic/kibana/assets/22172091/e6068e93-cf52-4c6c-a8ea-2d3dccb08491">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:ML/AIOps ML AIOps features: Change Point Detection, Log Pattern Analysis, Log Rate Analysis :ml release_note:enhancement v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants